-
Notifications
You must be signed in to change notification settings - Fork 88
Feature/jss 78 move our contract abis out of the constants package #963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/jss 78 move our contract abis out of the constants package #963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors contract ABI imports to use scoped subpath exports from @lit-protocol/contracts instead of the root package import, reducing bundle sizes by only including necessary network artifacts (datil/datil-dev/datil-test). A verification script is added to validate that bundles contain only the intended contract artifacts.
Key changes:
- Switched from root
@lit-protocol/contractsimport to scoped subpath imports for specific networks - Added bundle verification script to audit included contract artifacts
- Removed caret from contracts dependency version for stricter version control
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| verify-contracts-bundle.sh | New script that bundles test entries for constants and contracts-sdk packages, then analyzes which contract artifacts are included |
| packages/constants/src/lib/constants/mappers.ts | Refactored to import network-specific contract contexts via subpath exports instead of root import |
| package.json | Changed @lit-protocol/contracts dependency from ^0.0.74 to 0.0.74 (removed caret) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import { datil as datilContext } from '@lit-protocol/contracts/prod/datil.js'; | ||
| // @ts-ignore -- see note above. | ||
| import { datilDev as datilDevContext } from '@lit-protocol/contracts/prod/datil-dev.js'; | ||
| // @ts-ignore -- see note above. | ||
| import { datilTest as datilTestContext } from '@lit-protocol/contracts/prod/datil-test.js'; | ||
|
|
||
| type DatilContext = typeof datilContext; | ||
| type DatilDevContext = typeof datilDevContext; | ||
| type DatilTestContext = typeof datilTestContext; | ||
|
|
||
| const datil: DatilContext = datilContext; | ||
| const datilDev: DatilDevContext = datilDevContext; | ||
| const datilTest: DatilTestContext = datilTestContext; | ||
|
|
Copilot
AI
Oct 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These re-assignments from imported contexts to new constants appear unnecessary. The imports could be renamed directly using import { datil as datilContext } from ... and then used as datilContext throughout the code, or imported as datil directly to maintain the existing variable names. The current approach adds extra indirection without clear benefit.
| import { datil as datilContext } from '@lit-protocol/contracts/prod/datil.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilDev as datilDevContext } from '@lit-protocol/contracts/prod/datil-dev.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilTest as datilTestContext } from '@lit-protocol/contracts/prod/datil-test.js'; | |
| type DatilContext = typeof datilContext; | |
| type DatilDevContext = typeof datilDevContext; | |
| type DatilTestContext = typeof datilTestContext; | |
| const datil: DatilContext = datilContext; | |
| const datilDev: DatilDevContext = datilDevContext; | |
| const datilTest: DatilTestContext = datilTestContext; | |
| import { datil } from '@lit-protocol/contracts/prod/datil.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilDev } from '@lit-protocol/contracts/prod/datil-dev.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilTest } from '@lit-protocol/contracts/prod/datil-test.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like you prob need the typedefs here, and copilot is wrong?
glitch003
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very excited to see this!! thx for getting this in @Ansonhkg - will be a big improvement for devs. up to you if you wanna implement any of the copilot changes or not. the only one i saw that might make sense is https://github.com/LIT-Protocol/js-sdk/pull/963/files#r2441514234 but i don't think it really matters, just might be a little cleaner, but whatevs
MaximusHaximus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 this is a great improvement for code that does need the contracts-sdk (thus needs the ABIs) -- but it's not what the original goal was, and it still leaves 1MB of ABIs in the constants package for consumers who just import the constants package to get stuff like our LIT_CHAINS definitions, even though they don't need to use the ABIs. :)
The only consumer of the ABIs is the contracts-sdk, so moving the mapping into the contracts-sdk allowed us to eliminate all of the ABIs from simple consumers of the constants package. This is good because we often need constants in our LIT actions, but really don't want to bundle any of the LIT standard ABIs into the LIT action unless we actually need them 👍
The idea with this change was to eliminate any ABIs from being part of our core constants package at all, thus the ticket title of "move our contract ABIs our of the constants package", and the ticket description, and which is the PR title too, but this doesn't actually do...this change set is more like 'Don't import totally unused ABIs into the constants package' , which is still a great optimization, but not quite what we were looking for :). I think that we should mark this change as done under a different ticket so we don't lose track of the original goal of JSS-78 👍 We're up against LIT action size constraints in a major way w/ vincent actions so saving the other 1MB of ABIs that this leaves in the constants package would be a huge win 👍 :)
To recap, the idea is:
- If someone just needs
@lit-protocol/constants, they don't get any ABI mapping 🎉 - If someone needs the
contracts-sdk, then they will get the ABIs along with the contracts-sdk code...because they are a hard dependency requirement for the contracts-sdk :) - If someone wants JUST the ABIs they can import them directly from
@lit-protocol/contracts:)
| // import { datil, datilDev, datilTest } from '@lit-protocol/contracts'; | ||
|
|
||
| // @ts-ignore -- TypeScript can't resolve the subpath because this package compiles to CJS, but runtime bundlers can. | ||
| import { datil as datilContext } from '@lit-protocol/contracts/prod/datil.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just be able to import this without the .js extension and have TS detect the types properly 🤔
TS >4.9 uses the exports paths to find appropriate types 🤔
You might need to remove the barrel file from the root index.ts entirely 👍
We should also go ahead and remove the typingVersions export since our supported versions at this point is high enough that the exports key is what should be used anyway 👍 :)
WHAT
@lit-protocol/contractsexport map (legacy root import left commented for reference)verify-contracts-bundle.shscript to inspect bundle size forconstantsandcontracts-sdkpcakges for browser + node targetsAlso see: #962 (comment)
TEST
NPM
A snapshot has been published to
@lit-protocol/[email protected]Test repo: https://github.com/LIT-Protocol/pr-962-bundle-size-test
Before
https://test-962-contracts-sdk-bundle-size.vercel.app/bundle-report-lit-sdk.html
After
https://test-962-contracts-sdk-bundle-size.vercel.app/bundle-report-contracts-sdk-new.html
Locally
./verify-contracts-bundle.sh
BEFORE
AFTER